Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add subquery lowering #408

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add subquery lowering #408

wants to merge 1 commit into from

Conversation

alancai98
Copy link
Member

@alancai98 alancai98 commented Jul 8, 2023 •
edited
Loading

Lower subqueries to the logical plan's SubQueryExpr node. Targets the feat-bag-ops branch since both PRs adjust some query lowering code. Once #400 is merged, this PR will target main.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@alancai98 alancai98 self-assigned this Jul 8, 2023
@codecov
Copy link

codecov bot commented Jul 8, 2023 •
edited
Loading

Codecov Report

Patch coverage: 85.33% and project coverage change: +0.02 🎉

Comparison is base (d1bdfb0) 81.41% compared to head (0773ea0) 81.43%.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main     #408      +/-   ##
==========================================
+ Coverage   81.41%   81.43%   +0.02%     
==========================================
  Files          56       56              
  Lines       14912    14937      +25     
  Branches    14912    14937      +25     
==========================================
+ Hits        12140    12164      +24     
+ Misses       2251     2246       -5     
- Partials      521      527       +6
Impacted Files Coverage Δ
partiql-logical-planner/src/lower.rs 85.32% <84.93%> (-0.47%) ⬇️
partiql-eval/src/eval/evaluable.rs 90.27% <100.00%> (+0.81%) ⬆️

... and 3 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@github-actions
Copy link

github-actions bot commented Jul 8, 2023 •
edited
Loading

Conformance comparison report

Base (d1bdfb0) 9bb321e +/-
% Passing 84.12% 84.44% 0.32%
✅ Passing 5318 5338 20
❌ Failing 1004 984 -20
🔶 Ignored 0 0 0
Total Tests 6322 6322 0

Number passing in both: 5314

Number failing in both: 980

Number passing in Base (d1bdfb0) but now fail: 4

Number failing in Base (d1bdfb0) but now pass: 24

⁉️ CONFORMANCE REPORT REGRESSION DETECTED ⁉️. The following test(s) were previously passing but now fail:

Click here to see
  • partiql_tests::eval::query::group_by::group_by::group_by::simple_group_by::permissive_group_by_with_having_expression
  • partiql_tests::eval::query::group_by::group_by::group_by::simple_group_by::strict_group_by_with_having_expression
  • partiql_tests::eval::query::select::select::select_mysql::select::permissive_mysql_select_30
  • partiql_tests::eval::query::select::select::select_mysql::select::strict_mysql_select_30
The following test(s) were previously failing but now pass. Before merging, confirm they are intended to pass:
Click here to see
  • partiql_tests::eval::query::order_by::order_by::edge_cases::permissive_empty_projection_item_ordered_output_ordered
  • partiql_tests::eval::query::order_by::order_by::edge_cases::strict_empty_projection_item_unordered_output_unordered
  • partiql_tests::eval::spec_tests::section_11::strict_group_by_with_absent_values
  • partiql_tests::eval::query::select::select::select::select_distinct::strict_select_distinct_sub_query
  • partiql_tests::eval::spec_tests::section_11::strict_group_by_without_aggregates
  • partiql_tests::eval::query::order_by::order_by::edge_cases::strict_empty_projection_item_unordered_output_ordered
  • partiql_tests::eval::query::order_by::order_by::edge_cases::strict_empty_projection_item_ordered_output_ordered
  • partiql_tests::eval::query::select::select::from_clause::various_types_in_from_clause::permissive_range_over_nested_with_at
  • partiql_tests::eval::query::order_by::order_by::edge_cases::permissive_empty_projection_item_unordered_output_ordered
  • partiql_tests::eval::spec_tests::section_11::permissive_group_by_without_aggregates
  • partiql_tests::eval::query::select::select::select::select_distinct::strict_select_distinct_with_sub_query
  • partiql_tests::eval::spec_tests::section_11::strict_group_by_with_differenciated_absent_values
  • partiql_tests::eval::query::select::select::select::select_distinct::permissive_select_distinct_with_sub_query
  • partiql_tests::eval::query::select::select::sql_aggregate::sql_count::strict_aggregate_with_aliasing_in_subquery_of_select_value
  • partiql_tests::eval::spec_tests::section_6::strict_unpivot_with_pivot_to_analyze_attribute_names
  • partiql_tests::eval::query::select::select::from_clause::various_types_in_from_clause::strict_range_over_nested_with_at
  • partiql_tests::eval::spec_tests::section_11::permissive_group_by_with_differenciated_absent_values
  • partiql_tests::eval::spec_tests::section_6::permissive_unpivot_with_pivot_to_analyze_attribute_names
  • partiql_tests::eval::query::order_by::order_by::edge_cases::permissive_empty_projection_item_ordered_output_unordered
  • partiql_tests::eval::query::order_by::order_by::edge_cases::permissive_empty_projection_item_unordered_output_unordered
  • partiql_tests::eval::query::select::select::sql_aggregate::sql_count::permissive_aggregate_with_aliasing_in_subquery_of_select_value
  • partiql_tests::eval::spec_tests::section_11::permissive_group_by_with_absent_values
  • partiql_tests::eval::query::select::select::select::select_distinct::permissive_select_distinct_sub_query
  • partiql_tests::eval::query::order_by::order_by::edge_cases::strict_empty_projection_item_ordered_output_unordered

@alancai98 alancai98 force-pushed the feat-subquery-lower branch 2 times, most recently from a841aec to 64b07d4 Compare July 10, 2023 23:12
alancai98
alancai98 commented Jul 10, 2023
View reviewed changes
partiql-logical-planner/src/lower.rs
Comment on lines +625 to +627
// TODO: currently there's no enforcement for unique `OpId` between the outer query
// plan and the subquery plan. To decide if we want the `OpId`s to be unique
// between any subqueries.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The subquery plan and outer query plan don't currently enforce unique OpIds between the two plans. I was going off of how subqueries were modeled in the logical and eval plan as part of #227 and example tests.

I was wondering if we want unique OpIds between the two plans, which can be helpful for debugging and plan graph visualization.

@alancai98 alancai98 changed the title WIP subquery lowering Subquery lowering Jul 10, 2023
@alancai98 alancai98 changed the title Subquery lowering Add subquery lowering Jul 10, 2023
@alancai98 alancai98 marked this pull request as ready for review July 10, 2023 23:29
@alancai98 alancai98 requested review from jpschorr and am357 July 10, 2023 23:29
@alancai98 alancai98 force-pushed the feat-subquery-lower branch from 9366938 to dc518e8 Compare July 10, 2023 23:58
Base automatically changed from feat-bag-ops to main July 12, 2023 17:14
@alancai98 alancai98 force-pushed the feat-subquery-lower branch from dc518e8 to 0773ea0 Compare July 12, 2023 18:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jpschorr jpschorr Awaiting requested review from jpschorr

@am357 am357 Awaiting requested review from am357

At least 1 approving review is required to merge this pull request.

Assignees

@alancai98 alancai98

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
1 participant
@alancai98